test: drop best-effort catches from container-fixture teardown#85
Conversation
The integration-fixture DisposeAsync wrapped every container Dispose and the
SUT teardown in `try { ... } catch { /* best-effort */ }`, swallowing all
teardown faults to keep a failed InitializeAsync from being masked by a
secondary cleanup error.
That masking risk is gone: both DisposeSutAsync overrides already null-guard
their SUT (_factory / _host), so a failed init cannot NRE during cleanup. The
blanket catches only hid genuine teardown bugs from ever surfacing.
Replace them with plain null-guarded awaits, matching the Microsoft Agent
Framework test-fixture idiom (ConformanceTestBase / SessionPersistenceTests:
`client?.Dispose(); if (_app != null) await _app.DisposeAsync();`). Teardown
order is SUT-first (it consumes the containers) then infra; Testcontainers'
Ryuk reaper cleans up anything a mid-teardown throw leaves running. Also drop
the same swallow from SharedContainerFixture.DisposeSutAsync (_host.StopAsync).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
What
The integration-test
ContainerFixtureBase.DisposeAsyncswallowed every teardown fault behindtry { … } catch { /* best-effort */ }— one per container plus the SUT. The same blanket swallow lived inSharedContainerFixture.DisposeSutAsync(_host.StopAsync). This is a code smell: it hides real disposal/shutdown bugs forever.Replaced with plain null-guarded awaits — errors now surface.
Why the swallow was safe to delete
The catches existed to stop a failed
InitializeAsync(e.g. a Testcontainers wait-strategy timeout) from being masked by a secondary NRE during xUnit cleanup. That masking risk no longer exists: bothDisposeSutAsyncoverrides already null-guard their SUT —SharedRestContainerFixture:if (_factory is not null) await _factory.DisposeAsync();SharedContainerFixture:if (_host is not null) { … }so a failed init cannot NRE during cleanup. The blanket catches only suppressed genuine teardown faults. (xUnit v3 also aggregates lifecycle exceptions, so an init failure isn't lost even if teardown were to throw.)
Pattern source
Matches the Microsoft Agent Framework test-fixture idiom — plain null-guarded awaits, no best-effort catch — e.g.
ConformanceTestBase/SessionPersistenceTests:Teardown order is SUT-first (it consumes the containers) then infra; Testcontainers' Ryuk reaper cleans up anything a mid-teardown throw leaves running.
Verification
dotnet buildofPaperlessServices.Tests+PaperlessREST.Tests(→Paperless.TestSupport): Build succeeded, 0 warnings, 0 errors.Build & Test (backend)CI job — that is the runtime gate.Complete and compile-verified. No production code touched; test-fixture teardown only.
🤖 Generated with Claude Code